-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
arrayType #244 with tests #249
Conversation
@mourner any chance this can be reviewed and merged? |
I'm thinking now, it might be best to deal with this internally to supercluster. And importantly is it even worth bothering with Float32Array and just always use Float64Array instead? My benchmark tests showed now difference to memory usage, and the CPU, although the CPU time was 20% more (but I don't know enough about JS internals to know if that's due to this change). |
Yeah, I'm not sure I can merge this as is because adding an option for internal array type will only add confusion and complexity while sometimes introducing unintended consequences (such as twice the memory footprint). I don't see how it's possible for it to have no difference when Ideally the change would be internal, but to come up with the right fix, let's get a clear reproducible test case first. |
@ewelinaBS @andrewharvey can you please provide a jsbin that shows the problem? |
Yeah I don't understand, but I was just using the There will be a zoom level if we assume the closest point features still separated by radius * 2 where clustering with a radius would run into the Float32Array precision limit but not a Float64Array precision limit, and we could determine if our cluster maxzoom is greater than that zoom level to swap to Float64Array. Still I think it would be wise to get to the bottom of the memory situation for Float32Array / Float64Array here to verify that there is indeed a saving to be made. |
I'll close this, as it's clear this is most likely not the best solution going forward. Best to discuss the solution back at #243 |
I've added tests for #244